refactor(sdk,test-suite)!: convert to ESM, consume ESM dapi-client and wallet-lib#3683
refactor(sdk,test-suite)!: convert to ESM, consume ESM dapi-client and wallet-lib#3683PastaPastaPasta wants to merge 5 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "cb65af56c972a09767f118abb19fc931d30ec8a8e46494a5c05a0817626e9e6b"
)Xcode manual integration:
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The ESM conversion in js-dash-sdk is not self-consistent at this SHA. The new package boundary breaks existing in-repo deep imports, and the package is now published as ESM-only while at least one unchanged CommonJS runtime consumer still loads it with require(), so the monorepo contains executable paths that will fail after this change.
Reviewed commit: 68e85b41
🔴 2 blocking
GitHub's PR diff API returned PullRequest.diff too_large for this PR (>300 files), so these verified findings are posted in the top-level review body instead of inline comments.
1. blocking: The export map rewrites existing `dash/build/...` imports to non-existent paths
packages/js-dash-sdk/package.json (lines 7-9)
The new subpath export only defines "./*": "./build/*". For any existing import that already includes build/, Node substitutes the wildcard after ./, so dash/build/SDK/Client/Platform/signStateTransition.js resolves to ./build/build/SDK/Client/Platform/signStateTransition.js. This PR leaves live callers on that import form in packages/platform-test-suite/test/functional/platform/Document.spec.js:4, packages/platform-test-suite/test/functional/platform/Identity.spec.js:3, packages/platform-test-suite/test/e2e/withdrawals.spec.js:4, and packages/bench-suite/benchmarks/fees/dataContractCreate.js:1, so those paths stop resolving as soon as the export map is enforced.
Suggested replacement:
"exports": {
".": "./build/index.js",
"./build/*": "./build/*",
"./*": "./build/*"
}2. blocking: Switching `dash` to ESM-only breaks unchanged CommonJS consumers in the workspace
packages/js-dash-sdk/package.json (lines 5-9)
Adding "type": "module" makes build/index.js load as ESM, so require('dash') now throws instead of returning the SDK. This PR does not migrate all internal runtime consumers: packages/bench-suite/lib/client/createClientWithFundedWallet.js:1 still does const Dash = require('dash');, and packages/bench-suite/bin/bench.js is a CommonJS entrypoint that reaches that file. That leaves the benchmark path unusable at this SHA unless dash keeps a CommonJS entrypoint or all remaining CommonJS consumers are converted in the same change.
Note: reviewer session(s) with no parseable output: claude-general. The verifier checked the parseable agent findings against the checked-out SHA before this was posted.
68e85b4 to
dc7fb60
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I rechecked the incremental push at dc7fb608 against the current worktree. The latest push has the same tree as the prior reviewed commit 68e85b41, so there are no new delta-specific defects, but both prior blocking findings still reproduce at the current head and are carried forward below.
Reviewed commit: dc7fb608 (dc7fb6086f650b5d85a435b206d700868eeaaf30)
🔴 2 blocking
GitHub's PR diff API returned PullRequest.diff too_large for this PR (>300 files), so these verified findings are posted in the top-level review body instead of inline comments.
Prior finding reconciliation
- STILL VALID: prior #1 export map rewrites
dash/build/...imports to non-existent./build/build/...paths. Verified the package export map and live callsites still exist atdc7fb608. - STILL VALID: prior #2 ESM-only
dashstill breaks unchanged CommonJS workspace consumers. Verifiedbench-suiteremains CommonJS and still callsrequire('dash');wasm-drive-verifyalso still has arequire('dash')integration test.
New findings in latest delta
No new delta-specific findings. The latest push did not change the checked tree relative to 68e85b41; the review below carries forward the unresolved prior blockers.
1. blocking: Export map rewrites `dash/build/...` deep imports to non-existent `./build/build/...` paths
packages/js-dash-sdk/package.json (lines 7-10)
The exports block defines only ".": "./build/index.js" and the wildcard "./*": "./build/*". Under Node's subpath export pattern matching, a consumer importing dash/build/<sub> captures build/<sub> for * and resolves to ./build/build/<sub>, which does not exist on disk. The following imports still exist at this head and will fail to resolve:
packages/platform-test-suite/test/functional/platform/Identity.spec.js:3—import { createFakeInstantLock } from 'dash/build/utils/createFakeIntantLock.js';packages/platform-test-suite/test/functional/platform/Document.spec.js:4—import { signStateTransition } from 'dash/build/SDK/Client/Platform/signStateTransition.js';packages/platform-test-suite/test/e2e/withdrawals.spec.js:4—import { STATUSES as WITHDRAWAL_STATUSES } from 'dash/build/SDK/Client/Platform/methods/identities/creditWithdrawal.js';packages/bench-suite/benchmarks/fees/dataContractCreate.js:1—require('dash/build/SDK/Client/Platform/signStateTransition').
Since platform-test-suite is itself being migrated in this PR, this is a self-inconsistency: the new ESM tests cannot load through their own package boundary. Either drop the build/ prefix at the callsites or add an explicit "./build/*": "./build/*" mapping ahead of the catch-all so existing deep imports continue to resolve.
Suggested replacement:
"exports": {
".": "./build/index.js",
"./build/*": "./build/*",
"./*": "./build/*"
},2. blocking: ESM-only `dash` breaks unconverted CommonJS workspace consumers
packages/js-dash-sdk/package.json (lines 5-10)
Setting "type": "module" plus an exports map with no require condition makes require('dash') throw ERR_REQUIRE_ESM at runtime. The PR does not migrate every internal consumer:
packages/bench-suite/package.jsonhas no"type": "module"(verified — the package is still CommonJS), yetpackages/bench-suite/lib/client/createClientWithFundedWallet.js:1doesconst Dash = require('dash');and is reached from the CommonJSpackages/bench-suite/bin/bench.jsentrypoint. Thebenchscript is consequently unrunnable.packages/bench-suite/benchmarks/fees/dataContractCreate.js:1additionally usesrequire('dash/build/...'), compounding the breakage with the export-map issue above.packages/wasm-drive-verify/tests/integration/fetch_testnet_proofs.js:12usesrequire('dash'); this PR removes the last CJS-callable entrypoint, so the file can no longer loaddashwithout conversion.
Either convert the remaining CJS callers (bench-suite and the wasm-drive-verify integration test) in the same change, or expose a require-callable entrypoint via dual exports (e.g. ".": { "import": "./build/index.js", "require": "./build/index.cjs" }). The PR description only claims yarn workspace dash run test:unit passes; it does not address bench-suite, so the workspace is left in a broken state.
…ton/fetch/promisify shims
Non-breaking cleanup pass; package stays CJS, no public API changes, no consumer changes required.
dapi-client: replace winston with a minimal console-backed logger that preserves the same API (.error/.warn/.info/.verbose/.debug/.silly/.getForId). Drop node-fetch and the lib/test/bootstrap setimmediate shim — Node 18+ has both globally. Drop the https.Agent self-signed-TLS branch from requestJsonRpc (was Node-only; consumers wanting this must configure NODE_TLS_REJECT_UNAUTHORIZED at the app layer). Inline lodash/sample in ListDAPIAddressProvider. Add engines.node >=18.18. Remove dependencies: winston, node-fetch, lodash, bs58 (unused), node-inspect-extracted (unused). Remove devDeps: setimmediate.
dapi-grpc: inline the promisify shim in core/v0/web/CorePromiseClient.js and platform/v0/web/PlatformPromiseClient.js so the browser bundle no longer requires Node's util module. Both files document the shim so a future codegen regen does not silently reintroduce require('util').
… API
Adds lib/utils/bytes.js helper (hexToBytes/bytesToHex/base64ToBytes/bytesToBase64/concatBytes/bytesEqual) and converts all Buffer.* call sites in dapi-client lib/ to Uint8Array, with corresponding test updates. Package stays CJS.
Production exceptions where Buffer is retained: BlockHeadersReader passes Buffer to dashcore-lib's BlockHeader (its BufferReader needs .readInt32LE), and GetIdentitiesContractKeysResponse passes Buffer to wasm-dpp's Identifier.from (it explicitly requires Node Buffer).
createGrpcTransportError now handles both raw bytes (grpc-js path) and base64 strings (grpc-web path) for drive-error-data-bin, stack-bin, and dash-serialized-consensus-error-bin metadata fields, restoring the dual-format behavior that Buffer.from(x, 'base64') used to provide implicitly.
Test updates: spec files that construct expected protobuf requests now wrap .toBuffer() with new Uint8Array(...) to match production's normalization (sinon calledOnceWithExactly distinguishes Buffer from plain Uint8Array).
Breaking change for direct consumers: response object byte fields are now Uint8Array. Callers that do response.field.toString('hex') will fail — use bytesToHex(response.field) from lib/utils/bytes instead. Buffer.isBuffer(response.field) now returns false; use response.field instanceof Uint8Array.
Test results: dapi-client 315/315, wallet-lib 377/377, js-dash-sdk 60/60 — downstream consumers continue passing without modification (they exercise dapi-client mostly via mocks).
Converts @dashevo/dapi-client to pure ESM: type: module, exports map with ./lib/* wildcard, all CJS require/module.exports replaced with import/export, .js extensions on relative imports. Engine bumped to >=18.18. Drops webpack.config.js, karma.conf.js, and lib/test/karma/. Removes the @babel/core, babel-loader, browser-polyfill, karma, and webpack devDeps. Consumers must use a modern bundler (Vite/esbuild/webpack 5) which handles ESM natively. Moves 'events' npm package from devDependencies to dependencies — used by DAPIClient, ReconnectableStream, and BlockHeadersProvider for EventEmitter, and resolves correctly in both Node and browser bundlers. BREAKING: CJS consumers (require) no longer work. Downstream consumers wallet-lib (PR 4) and js-dash-sdk (PR 5) are temporarily broken between this PR merging and PRs 4/5 merging — they must land as a sequence. Dashmate is already ESM and continues to work. Test results: dapi-client 315/315. wallet-lib + js-dash-sdk fail as expected (fixed by PRs 4 + 5).
Converts @dashevo/wallet-lib to pure ESM so it can consume the ESM dapi-client from PR 3. Adds 'type: module' to package.json. All src/, fixtures/, and tests/ files converted from CJS require/module.exports to ESM import/export with .js extensions on relative imports. Deletes webpack.config.js, karma/, src/test/karma/. Removes browser-polyfill devDeps (buffer, crypto-browserify, stream-browserify, etc.) and webpack/karma. Tests run via Mocha in Node 18+; browser builds are out of scope. Engines: >=18.18. CJS-named-import fixes: lodash, crypto-js, @dashevo/dashcore-lib, @dashevo/wasm-dpp, @dashevo/grpc-common all use default-import + destructure pattern because Node ESM cannot statically enumerate named exports of CJS modules. Surfaces three previously-silent CJS bugs that ESM strict mode catches: missing UnknownStrategy export from errors/index.js, missing named exports for coinSelection strategies, and a 'type=' implicit-global in DerivableKeyChain.spec.js. Defensive (loadDpp.default ?? loadDpp)() unwrap in IdentitySyncWorker.onStart for the same wasm-dpp NodeNext interop reason as PR 3's bootstrap. Test results: wallet-lib 377/377 passing. dapi-client unchanged (315/315). js-dash-sdk still broken (PR 5 fixes).
…SM dapi-client and wallet-lib
Final PR in the ESM migration stack. Converts the two remaining workspace packages that consumed dapi-client/wallet-lib, restoring monorepo-wide green tests after PR 3.
js-dash-sdk: TypeScript-based package with NodeNext module resolution. Adds 'type: module' + exports map. tsconfig.build.json → module: nodenext, moduleResolution: nodenext, target: es2020. All .ts files get .js extensions on relative imports (TS NodeNext compiles .ts to .js but the import string must already say .js). Drops ts-mock-imports by refactoring register.ts to accept an injectable randomBytes parameter (ts-mock-imports relies on mutable module exports which ESM forbids). Deletes webpack/karma + browser polyfills. Replaces winston with console-backed logger. CJS-named-import conversions for @dashevo/dashcore-lib, @dashevo/wasm-dpp, @dashevo/grpc-common, and the system-id imports from contract packages. Adds parallel 'import type' aliases (via 'import type { X as XType }; type X = XType;') in 22 .ts files where destructured value names are also used as types — works around TS2440 namespace collision under NodeNext.
platform-test-suite: converted ~35 files from CJS to ESM. Test bootstrap converted; .mocharc.yml uses 'import:' for ESM root hooks.
Defensive (loadDpp.default ?? loadDpp)() unwrap in 4 sites (Platform.initializeDppModule, createIdentityFixtureInAccount, contracts/get.spec.ts, contracts/history.spec.ts). createGrpcTransportError-style dual-format byte handling preserved.
Test results: 752 passing across the monorepo (dapi-client 315, wallet-lib 377, js-dash-sdk 60). platform-test-suite is end-to-end and requires live testnet credentials in .env (pre-existing constraint).
dc7fb60 to
4e44ff6
Compare
|
Closing — combined into #3689 because the three are lockstep (CI fails on any in isolation since CJS can't require ESM, leaving the monorepo in a non-buildable mid-state). The three commits are preserved on that branch for focused review. |
Summary
Final PR in the ESM migration stack. Converts
dash(@dashevo/js-dash-sdk) and@dashevo/platform-test-suiteto ESM. Restores monorepo-wide green tests after PR 3.This is PR 5 of 5. Depends on #3679, #3680, #3681, #3682.
What changes
js-dash-sdk
package.json:"type": "module",exportsmap,engines.node >= 18.18. Dropsunpkg/browser,test:browsers*,prepublishOnly. Removes devDeps: allkarma-*,webpack, browser polyfills,ts-mock-imports(replaced).tsconfig.build.jsonandtsconfig.json:module: nodenext,moduleResolution: nodenext,target: es2020..tssource files get.jsextensions on relative imports (TS NodeNext requirement: TS compiles.tsto.js, but the import string must already say.js).src/SDK/Client/Platform/methods/names/register.tsto accept an injectablerandomBytesparameter instead of relying onts-mock-imports. ESM module exports are frozen;ts-mock-imports-style export mutation no longer works.winstonwith console-backed logger.@dashevo/dashcore-lib,@dashevo/wasm-dpp,@dashevo/grpc-common, and the system-id imports from contract packages (@dashevo/dpns-contract,@dashevo/dashpay-contract, etc.).import type { X as XType }; type X = XType;aliases in 22.tsfiles where destructured value names are also used in type positions. Works around TS2440 namespace collision undermodule: nodenext.(loadDpp.default ?? loadDpp)()unwrap in 4 sites where wasm-dpp's CJS default is called directly (Platform.initializeDppModule,createIdentityFixtureInAccount, two contract specs).webpack.config.js,webpack.base.config.js,karma/.platform-test-suite
package.json:"type": "module", dropstest:browsers, browser polyfills,karma-*..jsfiles converted to ESM with.jsextensions on relative imports.lib/test/bootstrap.jsrewritten as ESM withfileURLToPath-based__dirname..mocharc.ymlusesimport:for ESM root hooks.karma.conf.js,lib/test/karma/.Test plan
yarn workspace @dashevo/dapi-client run test:unit— 315 passing ✅yarn workspace @dashevo/wallet-lib run test:unit— 377 passing ✅yarn workspace dash run test:unit— 60 passing ✅ (3 pending pre-existing)yarn workspace @dashevo/platform-test-suite run lint— ESM bootstrap loads.env(pre-existing constraint, unrelated to this conversion)Breaking changes
require('dash')andrequire('@dashevo/platform-test-suite')no longer work..jsextensions.Platform.names.register(...)gained an optional trailingrandomBytesparameter. Existing callers unaffected; subclasses or replacements need to know.winston,bs58,node-inspect-extractedfrom deps where unused.Monorepo state after this stack lands
@dashevo/dapi-client— ESM,Uint8ArrayAPI, browser-bundler-friendly (Vite/esbuild/webpack 5)@dashevo/wallet-lib— ESMdash— ESM TypeScript@dashevo/platform-test-suite— ESM@dashevo/dashmate— was already ESM, works as-is@dashevo/dapi-grpc— patched in PR 1; rest of package unchangedStack